fix(events): route the module-level events.* helpers through the dynamic dispatch - #7745
Conversation
📝 WalkthroughWalkthroughModule-level ChangesEvents dynamic dispatch
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant nm_dispatch_events
participant js_events_native_dispatch
participant EventsHelpers
Caller->>nm_dispatch_events: invoke dynamic events helper
nm_dispatch_events->>js_events_native_dispatch: forward method and arguments
js_events_native_dispatch->>EventsHelpers: route to matching helper
EventsHelpers-->>js_events_native_dispatch: return result
js_events_native_dispatch-->>nm_dispatch_events: return NaN-boxed value or undefined
nm_dispatch_events-->>Caller: return dispatched result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Validation complete — out of draftnode-suite
The single base-arm failure is the new fixture, which is the point: it is red without the bridge and green with it. The 4 crashes are byte-identical across arms and pre-existing — Unit tests
One process noteMy first A/B of this change was vacuous and I nearly believed it: I swapped the compiler binary while both arms linked the same |
…mic dispatch `events.listenerCount(e, "x")` returned 2 while `const c = events.listenerCount; c(e, "x")` returned `undefined`. Same for `(events as any).listenerCount(...)` and `events.listenerCount(...args)`. `nm_dispatch_events` had arms only for `init` and `EventEmitterAsyncResource`; everything else fell to `_ => undefined`. The seven module-level helpers (`listenerCount`, `once`, `on`, `getEventListeners`, `getMaxListeners`, `setMaxListeners`, `addAbortListener`) are implemented in perry-stdlib, which depends on perry-runtime and so cannot be named from the dispatch bucket. The static call reaches them directly through the codegen `NativeModSig` rows, which is why only the indirect forms were dead. Add the registered-pointer bridge every comparable module already has (zlib / querystring / domain): `JS_NATIVE_EVENTS_DISPATCH` + `js_set_native_events_dispatch` in perry-runtime, `js_events_native_dispatch` in perry-stdlib, wired at `js_stdlib_init_dispatch`. Argument marshalling matches what the static path's `NA_STR` / `NA_VARARGS` rows produce: event names go through ToString, and `setMaxListeners(n, ...targets)` rebuilds its trailing targets into the single array the helper expects. Distinct from the existing `JS_NATIVE_EVENTS_CONSTRUCT`, which serves only `new`. Follow-up to #7734, which named this as the one remaining wrong-to-wrong case in the #7720 spread-call matrix: `events.listenerCount(...args)` turned a bogus ERR_INVALID_ARG_TYPE throw into `undefined`. It now returns the count. Tests: `events_dispatch_parity_tests` walks the real `NET_EVENTS_ROWS` table and fails on any `has_receiver: false` `events` row that is not classified as routed-to-stdlib, answered-by-runtime, or deliberately-unrouted — the drift that caused this bug — plus a stale-entry check so the classification cannot rot. Both halves sabotage-checked. `node-suite/events/listeners/ module-helper-dynamic-dispatch.ts` byte-compares the static, captured, type-erased and spread forms against node. `events.on`'s async ITERATION is deliberately not asserted: it drops its first value in the STATIC form too, on a tree without this change (`events/on/ async-iterator-abort` and `events/on/validation` are already red for it). The helper is routed all the same, so it inherits the fix when that gap closes.
cc1aa6c to
d5da0af
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-stdlib/src/events/module_helpers.rs`:
- Around line 237-243: Update event_name_header to create a RuntimeHandleScope
and root the input value before calling either conversion helper. Keep the
resulting materialized string rooted through the selected return path, including
the js_jsvalue_to_string fallback, so GC-safe handles are passed across
allocating or user-code-invoking calls.
In `@test-parity/node-suite/events/listeners/module-helper-dynamic-dispatch.ts`:
- Line 16: Update the fixture around countArgs to use a coercible non-string
event name, and expand the dynamic/spread setMaxListeners invocation to pass two
distinct EventEmitter targets so both targets are exercised.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c14e1cda-7a00-47fb-ae19-4c30785862a1
📒 Files selected for processing (12)
changelog.d/7745-events-module-helper-dynamic-dispatch.mdcrates/perry-codegen/src/lower_call/native_table/events_dispatch_parity_tests.rscrates/perry-codegen/src/lower_call/native_table/mod.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/object/native_module_dispatch/dispatch_d_i.rscrates/perry-runtime/src/value/handle.rscrates/perry-runtime/src/value/mod.rscrates/perry-runtime/src/value/tags.rscrates/perry-stdlib/src/common/dispatch/init.rscrates/perry-stdlib/src/events.rscrates/perry-stdlib/src/events/module_helpers.rstest-parity/node-suite/events/listeners/module-helper-dynamic-dispatch.ts
| unsafe fn event_name_header(value: f64) -> *const StringHeader { | ||
| let materialized = perry_runtime::string::js_string_materialize_to_heap(value); | ||
| if !materialized.is_null() { | ||
| return materialized; | ||
| } | ||
| perry_runtime::value::js_jsvalue_to_string(value) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Root the event-name value during coercion.
Line 242 calls js_jsvalue_to_string(value) while value exists only in an unrooted Rust local. Object coercion can allocate or invoke user code. GC can relocate the NaN-boxed source value during that call.
Use RuntimeHandleScope to root the input before conversion. Keep the materialized string rooted until the selected helper consumes it.
Based on learnings: production GC does not scan Rust stack locals; root NaN-boxed values across allocating or user-code-invoking calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-stdlib/src/events/module_helpers.rs` around lines 237 - 243,
Update event_name_header to create a RuntimeHandleScope and root the input value
before calling either conversion helper. Keep the resulting materialized string
rooted through the selected return path, including the js_jsvalue_to_string
fallback, so GC-safe handles are passed across allocating or user-code-invoking
calls.
Source: Learnings
|
|
||
| const dyn: any = events; | ||
| const captured = events.listenerCount; | ||
| const countArgs: [EventEmitter, string] = [e, "x"]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files around candidate =="
git ls-files | rg 'module-helper-dynamic-dispatch|module-helper|emit|setMaxListeners|events/listeners' | sed -n '1,200p'
echo
echo "== candidate file =="
if [ -f test-parity/node-suite/events/listeners/module-helper-dynamic-dispatch.ts ]; then
cat -n test-parity/node-suite/events/listeners/module-helper-dynamic-dispatch.ts
fi
echo
echo "== related helper definitions/usages =="
rg -n "setMaxListeners|EventEmitter|module-helper|dynamic dispatch|emit" test-parity -S -n | sed -n '1,240p'Repository: PerryTS/perry
Length of output: 38581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== event-name-coercion.ts =="
cat -n test-parity/node-suite/events/listeners/event-name-coercion.ts | sed -n '1,140p'
echo
echo "== max-listeners target validation =="
cat -n test-parity/node-suite/events/max-listeners/module-helper-target-validation.ts | sed -n '1,180p'
echo
echo "== event-helper dispatch related source =="
rg -n "nm_dispatch_events|setMaxListeners|listenerCount|getEventListeners|js_jsvalue_to_string_coerce|coerce|targets|EventEmitter" crates/perry-codegen-wasm/src/emit crates/perry-stdlib crates/perry-runtime -S | sed -n '1,240p'
echo
echo "== focused files likely containing dispatch implementation =="
git ls-files | rg 'crates/perry-codegen-wasm/src/emit/(calls|exprs_more|module_emitter|func_emit_ctx|helpers|native).rs|crates/perry-stdlib/src/common/dispatch/emitter_als.rs' | xargs -r -n1 sh -c 'echo "--- $0"; wc -l "$0"; sed -n "1,240p" "$0"'Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== change summary =="
git diff --stat HEAD~1..HEAD || true
echo
echo "== event helper dynamic dispatch diff snippets =="
git diff HEAD~1..HEAD -- test-parity/node-suite/events/listeners/module-helper-dynamic-dispatch.ts crates/perry-stdlib/src/common/dispatch/emitter_als.rs crates/perry-codegen-wasm/src/emit/calls.rs crates/perry/runtime/src/events -U 80 | sed -n '1,260p'
echo
echo "== setMaxListeners implementation candidates =="
rg -n "js_event_emitter_set_max_listeners|setMaxListeners|event_names|listener_count|set.*max_listener|maxListeners" crates/perry-runtime/src crates/perry-stdlib/src/events crates/perry-stdlib/src/common/dispatch -S | sed -n '1,220p'Repository: PerryTS/perry
Length of output: 21826
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== event module helpers =="
sed -n '1,220p' crates/perry-stdlib/src/events/module_helpers.rs | cat -n
echo
echo "== events helper dispatch call args construction =="
rg -n "js_events_listener_count|js_events_set_max_listeners|js_events_get_max_listeners|event_name_header|args_array|rest_arr|array_from_values|pack_args_array" crates/perry-codegen-wasm/src/emit/calls.rs crates/perry-codegen-wasm/src/emit/expr/ -S | sed -n '1,240p'
echo
echo "== static dispatch branch construction =="
sed -n '200,330p' crates/perry-stdlib/src/events/module_helpers.rs | cat -n
echo
echo "== dynamic dispatch emitter_als relevant sections =="
sed -n '100,170p' crates/perry-stdlib/src/common/dispatch/emitter_als.rs | cat -nRepository: PerryTS/perry
Length of output: 11207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== event module helpers tail =="
sed -n '220,360p' crates/perry-stdlib/src/events/module_helpers.rs | cat -n
echo
echo "== helper target and event bits utilities =="
rg -n "enum EventHelperTarget|struct EventEmitterHandle|max_listeners|fn event_bits_from_string_ptr|fn event_name_header|fn event_bits|js_events_function" crates/perry-runtime crates/perry-stdlib -S | sed -n '1,220p'
echo
echo "== calls.rs file list nearby and content =="
git ls-files 'crates/perry-codegen-wasm/src/emit/*.rs' | sed -n '1,80p'
for f in crates/perry-codegen-wasm/src/emit/calls.rs crates/perry-codegen-js/src/emit/calls.rs crates/perry-codegen-js/src/emit/exprs_more.rs; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n "js_events_listener_count|js_events_get_max_listeners|js_events_set_max_listeners|event_name_header|pack_args_array|rest_arr|dynamic|variadic|arg\(0\)|arg\(1\)|args\[1\]|args" "$f" -S | sed -n '1,200p'
fi
doneRepository: PerryTS/perry
Length of output: 23088
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== wasm native method call emitting =="
rg -n "native_table|NativeCall|emit_native|js_events_native_dispatch|js_events.*_call|js_array_alloc|pack_args_array|rest" crates/perry-codegen-wasm/src/emit crates/perry-runtime/src -S | sed -n '1,260p'
echo
echo "== native table entries for events =="
rg -n '"events"|NativeModSig|events::|setMaxListeners|listenerCount|getMaxListeners|event_name_header|NA_VARARGS|NA_STR' crates/perry-codegen-wasm/src crates/perry-codegen-js/src crates/perry-codegen-glance/src crates/perry-codegen-arkts/src -S | sed -n '1,320p'Repository: PerryTS/perry
Length of output: 27973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== native.rs around events =="
sed -n '220,310p' crates/perry-codegen-js/src/emit/native.rs | cat -n
echo
echo "== native export table for events helpers =="
sed -n '280,360p' crates/perry-runtime/src/object/native_module/callable_export_table.rs | cat -n
echo
echo "== native call implementation around calls array construction =="
rg -n "js_array_alloc\\(2\\)|array_from_values|pack_args_array|js_native_call_" crates/perry-runtime/src/closure crates/perry-runtime/src/object/native_module crates/perry-std -S | sed -n '1,260p'Repository: PerryTS/perry
Length of output: 9423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== js_native_call_method definitions =="
rg -n "extern \"C\" fn js_native_call_method|fn js_native_call_method|js_native_call_method_apply|js_native_call_method_array|arguments_array|rest_array|args_array|js_array_alloc|pack_args_array" crates/perry-runtime/src/closure crates/perry-runtime/src/object crates/perry-runtime/src -S | sed -n '1,240p'
echo
echo "== dispatcher native exports call dispatch =="
sed -n '540,630p' crates/perry-runtime/src/object/native_module/callable_exports.rs | cat -n
rg -n "js_native_call_value\\(|js_native_call_method\\(|callable|arg\\(" crates/perry-runtime/src/closure/dispatch.rs crates/perry-runtime/src/closure/dispatch/value_call.rs -A 8 -B 8 | sed -n '1,240p'Repository: PerryTS/perry
Length of output: 50369
Cover non-string event names and multiple setMaxListeners targets.
This fixture still only exercises a primitive string event name. Also, setMaxListeners is only called once per emitter; a branch that applies the limit to only the first target would pass this case. Add a coercible event-name call and call dynamic/spread setMaxListeners with two distinct targets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-parity/node-suite/events/listeners/module-helper-dynamic-dispatch.ts` at
line 16, Update the fixture around countArgs to use a coercible non-string event
name, and expand the dynamic/spread setMaxListeners invocation to pass two
distinct EventEmitter targets so both targets are exercised.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 318: Restore the release metadata to version 0.5.1434: update
[workspace.package].version in Cargo.toml at lines 318-318 and Current Version
in CLAUDE.md at lines 11-11. Do not retain the feature-PR bump to 0.5.1435.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b09ff5d9-12d8-4f53-9371-4276850e01dc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
CLAUDE.mdCargo.toml
|
|
||
| [workspace.package] | ||
| version = "0.5.1434" | ||
| version = "0.5.1435" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Cargo.toml relevant =="
if [ -f Cargo.toml ]; then
sed -n '1,45p;305,325p' Cargo.toml
else
echo "Cargo.toml not found"
fi
echo
echo "== CLAUDE.md relevant =="
if [ -f CLAUDE.md ]; then
sed -n '1,25p' CLAUDE.md
else
echo "CLAUDE.md not found"
fi
echo
echo "== git diff stats =="
git diff --stat || true
echo
echo "== version lines in changed files =="
git diff -- Cargo.toml CLAUDE.md | sed -n '/@@ .*Cargo.toml/,/@@ .*CLAUDE.md/p; /@@ .*CLAUDE.md/,$p' | grep -nE '(^\+|^\-).*version|^\+|^\-Current Version' || true
echo
echo "== current branch / upstream hints =="
git branch --show-current || true
git rev-parse --abbrev-ref HEAD || true
git rev-parse HEAD || trueRepository: PerryTS/perry
Length of output: 5260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local git status =="
git status --short
echo
echo "== show current branch HEAD commit subject/timestamp =="
git log -1 --pretty=format:'%H%n%ad %s' --date=format:%Y-%m-%d || true
echo
echo "== inspect possible release metadata guidance =="
for f in contributing.md CONTRIBUTING.md README.md CLAUDE.md; do
if [ -f "$f" ]; then
echo "-- $f --"
rg -n "version|versioning|release|version updates|Current Version|workspace\.package" "$f" || true
fi
done
echo
echo "== search repository for release/version metadata instructions =="
rg -n "version updates|release metadata|workspace\.package\.version|Current Version|increment the patch version|main.*version|version" . \
-g '!target/**' -g '!dist/**' -g '!build/**' | head -n 120 || trueRepository: PerryTS/perry
Length of output: 26924
Do not bump release metadata in this feature PR.
This commit bumps [workspace.package].version in Cargo.toml and Current Version in CLAUDE.md to 0.5.1435. Those version updates are reserved for the maintainer release path; contributors should not change them in a feature PR. Restore both values to 0.5.1434.
📍 Affects 2 files
Cargo.toml#L318-L318(this comment)CLAUDE.md#L11-L11
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` at line 318, Restore the release metadata to version 0.5.1434:
update [workspace.package].version in Cargo.toml at lines 318-318 and Current
Version in CLAUDE.md at lines 11-11. Do not retain the feature-PR bump to
0.5.1435.
Sources: Coding guidelines, Learnings
Merging as v0.5.1435The best thing in this PR is that you caught your own vacuous A/B. This is a runtime change, so swapping only the compiler binary compares the same A "no change" result from an A/B that could not have shown a change is worse than no measurement, because it reads as evidence. Third instance of one shape, and worth naming as a class
The structural cause is the same both times and is stated correctly: the module-level helpers live in perry-stdlib, which depends on perry-runtime, so the dispatch bucket cannot name them. The static call reaches them through codegen's Given three instances, the general question is worth asking separately: which other modules advertise names their bridge does not implement? That is mechanically checkable — enumerate each The fix follows the established pattern
Argument marshalling matching what the static path's rows produce is the detail that would otherwise bite: event names through Nine behaviours verified against node 26.5.1 across captured / dynamic / spread forms, including Gates 21/21. |
1 participant
Closes the one item #7734 left open. Draft until the node-suite A/B and the full
perry-codegenrun finish — both are in flight; numbers go in a comment.The bug
nm_dispatch_eventshad arms for exactly two names —initandEventEmitterAsyncResource— and everything else fell to_ => undefined. The seven module-level helpers live in perry-stdlib, which depends on perry-runtime, so the dispatch bucket cannot name them; the static call reaches them directly through codegen'sNativeModSigrows, which is why only the indirect forms were dead.This is the third instance of one shape. #7734 fixed it for
querystring(bridge implemented 1 of 7 advertised names); here the bridge did not exist at all.The fix
The registered-pointer bridge every comparable module already has (zlib / querystring / domain / tls):
JS_NATIVE_EVENTS_DISPATCH+js_set_native_events_dispatch, and an arm innm_dispatch_eventsrouting the seven namesjs_events_native_dispatch, wired atjs_stdlib_init_dispatchArgument marshalling matches what the static path's rows produce: event names go through
ToString(theNA_STRcoercion), andsetMaxListeners(n, ...targets)rebuilds its trailing targets into the single array the helper expects (NA_VARARGS). Distinct from the existingJS_NATIVE_EVENTS_CONSTRUCT, which serves onlynew.Measured
A/B with two different
libperry_{runtime,stdlib}.apairs — this is a runtime change, so swapping only the compiler binary would have compared the same archive against itself and reported a vacuous "no change" (it did, on the first attempt; the archives are now staged separately and verifiedcmp-different):listenerCountcaptured / dynamic / spreadundefinedgetEventListenersdynamicTypeErrorgetMaxListenersdynamicundefinedsetMaxListenersdynamic / spreadoncedynamic / spreadundefined[42]/[7][42]/[7]addAbortListenerdynamicTypeError: is not iterableundefinedundefinedundefinedTests
events_dispatch_parity_tests(perry-codegen,--lib, so per-PR visible) walks the realNET_EVENTS_ROWStable and fails on anyhas_receiver: falseeventsrow that is not classified as routed-to-stdlib / answered-by-runtime / deliberately-unrouted. That is exactly the drift that caused this bug: a static row landed with no dynamic counterpart and nothing noticed. A second test rejects stale classification entries, so the list cannot rot into a rubber stamp. Both halves sabotage-checked (drop a name → first test fails; add a phantom → second fails).node-suite/events/listeners/module-helper-dynamic-dispatch.tsbyte-compares static / captured / type-erased / spread forms against node.Deliberately not asserted
events.on's async iteration.for await (const v of events.on(e, "tick"))drops its first value in the static form too, on a tree without this change —events/on/async-iterator-abortandevents/on/validationare already red for it, and I confirmed the static form fails identically on the unpatched archive. Asserting it in this fixture would test that bug rather than this one.events.onis routed by the bridge all the same, so it inherits the fix when that gap closes.Summary by CodeRabbit
Bug Fixes
node:eventshelper calls.setMaxListenerstarget arrays.once,on, and abort-listener helpers.undefined.Tests